fix(branch): forward --review-provider to the merge gate - #689
Conversation
`gx ship` / `gx finish` honor `--review-provider` — args.js parses it and
finish/index.js hands the whole options object to runReviewGate. `gx branch
finish --gate-review` passed a bare `options: {}`, so the gate always ran
review-gate.js's `codex` default no matter what the caller asked for.
On a machine where codex is unavailable that makes the gate unusable: it fails
closed on every run, and the only way to land anything is --skip-review-gate —
the gate switched off by the very mechanism meant to enforce it. Which is the
same failure mode the --gate-review routing (#677) was added to prevent.
The flag and its value are consumed here rather than passed through, because
agent-branch-finish.sh exits 1 on the unknown argument (and would otherwise see
a bare "claude" positional). An unknown provider throws before the script runs,
so a typo cannot silently downgrade to the default.
A bare --gate-review is unchanged: reviewProvider is undefined and
review-gate.js applies its own default.
args.js throws "requires a value of codex|claude" when the value is missing; branch.js silently left reviewProvider undefined, so the gate fell back to its codex default. A caller who asked for claude and mistyped would be gated by codex without being told — the same silent-fallback failure this change set out to remove. A missing value now becomes "" so the existing validation rejects it.
NagyVikt
left a comment
NagyVikt
left a comment
There was a problem hiding this comment.
Review — adversarial pass over the diff
Decision: APPROVE. 0 CRITICAL, 0 HIGH. One MEDIUM was found during the pass and is fixed in b411c18, before merge.
Found and fixed in this PR
--review-provider with no value fell back silently (b411c18)
args.js throws --review-provider requires a value of codex|claude when the value is missing. The first commit left reviewProvider as undefined in that case, so the gate quietly used its codex default — a caller who asked for claude and mistyped would be gated by codex with no signal. That is the same silent-fallback failure this PR exists to remove. A missing value now becomes "" so the existing validation rejects it.
Checked, no finding
- Argv consumption — the
index += 1inside the--review-providerbranch correctly skips the value; the loop increment then moves past it. Pinned bydeepEqualassertions that neither the flag nor its value reaches the script argv (agent-branch-finish.shexits 1 on both). - Backward compatibility — a bare
--gate-reviewleavesreviewProviderundefined andreview-gate.jsapplies its own default, so existing callers are unchanged. Pinned by the pre-existing tests, all still green. - Fail-closed ordering — validation throws inside
splitGateReviewFlags, i.e. beforeinvokePackageAsset('branchFinish', ...), so a bad provider cannot reach the merge. Asserted (calls.script.length === 0). - Inline form —
--review-provider=claudehandled;--review-provider=(empty) throws. - Security / injection — the value is compared against a fixed allowlist and never interpolated into a shell command.
Known, deliberate divergence (LOW)
branch.js normalises with .trim().toLowerCase(); args.js compares the raw token. So --review-provider CLAUDE is accepted here and rejected by gx ship. The divergence is a superset — it can never select a provider the caller did not ask for — so it is left as-is rather than tightening a working path. Worth unifying if the two parsers are ever consolidated.
Validation
| Check | Result |
|---|---|
Targeted tests (test/branch-gate-review.test.js) |
12/12 pass — 4 new (flag form, inline form, invalid value, missing value). RED before the fix. |
| Full suite | 802 tests, 41 failing — failing set byte-identical to main, verified by stashing the change and re-running; diff of the sorted not ok names is empty. No new failures. |
| CI | No checks registered on this PR. |
Disclosure
This is a self-review by the change's author, not an independent one. It is recorded here because the repo has no required checks and no review bot on this PR, so nothing else would document the reasoning.
1 participant
gx branch finish --gate-reviewpassedoptions: {}torunReviewGate, so the gate always used review-gate.js'scodexdefault and ignored--review-provider.gx ship/gx finishhonor the flag;gx branch finishdid not.Where codex is unavailable this makes the gate unusable — it fails closed on every run and the only way forward is
--skip-review-gate, i.e. the gate disabled by the mechanism meant to enforce it. Same failure mode #677 was added to prevent.Changes
splitGateReviewFlagsconsumes--review-provider <v>and--review-provider=<v>(the value too —agent-branch-finish.shexits 1 on the unknown flag and would otherwise see a bareclaudepositional).--gate-reviewunchanged:reviewProvideris undefined and review-gate.js applies its own default.Test plan
test/branch-gate-review.test.js(flag form, inline form, invalid value). RED before the fix, GREEN after: 11/11.main(verified by stashing the change and re-running;diffempty). No new failures.